Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix tests for current Julia nightly #529

Merged
merged 3 commits into from
Apr 20, 2018
Merged

Fix tests for current Julia nightly #529

merged 3 commits into from
Apr 20, 2018

Conversation

martinholters
Copy link
Member

The first commit replaces calls to CartesianIndices(1:3, 1:2) with CartesianIndices((Base.OneTo(3), Base.OneTo(2))). The former now gives an error when used for indexing, e.g.:

julia> zeros(3, 2)[CartesianIndices((1:3, 1:2))]
ERROR: MethodError: no method matching similar(::Array{Float64,2}, ::Type{Float64}, ::Tuple{UnitRange{Int64},UnitRange{Int64}})

Is that supposed to happen, @mbauman?

The second commit removes the testfindfirst on IOBuffer as Julia removed support for it in JuliaLang/julia#26600. Should we also add a deprecated method to findfirst, using the definition of the deprecated search, i.e. https://github.com/JuliaLang/julia/blob/278a2893b8b02d6063ee4b1717c201c3431cb042/base/deprecated.jl#L1565-L1571 ?

@martinholters
Copy link
Member Author

Looks like I should have checked on current current Julia nightly... Will look into the remaining failures.

Meanwhile CC @nalimilan for the findfirst thing.

@nalimilan
Copy link
Member

Thanks. I don't think we should add a deprecation to findfirst given that we only define the method on Julia 0.6. If we deprecated it there packages would start printing annoying warnings even for users who don't care about 0.7.

Regarding CartesianIndices, why not use CartesianIndices((1:3, 1:3)) instead of OneTo?

@martinholters
Copy link
Member Author

Regarding CartesianIndices, why not use CartesianIndices((1:3, 1:3)) instead of OneTo?

JuliaLang/julia#26682 (comment)

@martinholters martinholters mentioned this pull request Apr 10, 2018
Merged
@martinholters
Copy link
Member Author

The remaining failure is not just a problem with the tests. So I propose we merge this as is, which will enable a future PR fixing round and friends to actually see its tests run and get a green CI.

@martinholters martinholters mentioned this pull request Apr 10, 2018
Merged
@nalimilan
Copy link
Member

I'd rather wait until we have a clearer view of what we want to do, or disable/remove the map(i -> l[i], c) test, as I'm not sure it's supposed to work at all. I've added it when backporting CartesianIndices, but it's not tested in Base obviously, so it's fine if Compat doesn't test it either (at least for now). Using Base.OneTo makes less sense to me since we mainly want to test ordinary ranges, which users are more likely to use.

@martinholters
Copy link
Member Author

I'd rather wait until we have a clearer view of what we want to do, or disable/remove the map(i -> l[i], c) test

Yep, will do. But note that it's also the l[c] that fails.

@martinholters martinholters mentioned this pull request Apr 20, 2018
Merged
@martinholters
Copy link
Member Author

Ah, test failures on the out-of-data Julia nightlies are due to a mistake in #530. I'll add the fix to this PR to verify I get CI green...

@martinholters
Define Compat.round(x, digits; base = base) on all versions of Julia
eec1e2c 
Even where `Base` has these methods, we still need to provide them as
methods to the corresponding `Compat` functions.
@martinholters
Copy link
Member Author

Good to go?

@martinholters martinholters merged commit ab62df3 into master Apr 20, 2018
@martinholters martinholters deleted the mh/fixtests branch April 20, 2018 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fredrikekre fredrikekre fredrikekre approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@martinholters @nalimilan @fredrikekre